Fix handling of LoggerContextAware lookups
#2313
Conversation
ppkarwasz
added
the
bug
|
@ppkarwasz, thanks for considering me for the review. Unfortunately I am not much familiar with the parts you touch, hence I doubt if my review will be meaningful. I trust your judgement and suggest to proceed as you wish. |
| public void setConfiguration(final Configuration configuration) { | ||
| super.setConfiguration(configuration); | ||
| // Propagate | ||
| for (final StrLookup lookup : strLookupMap.values()) { | ||
| if (lookup instanceof ConfigurationAware) { | ||
| ((ConfigurationAware) lookup).setConfiguration(configuration); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@vy,
This is the part I wanted to consult you on. IMHO it is bad practice when methods called set* do anything else than set a single value.
I think in this case the usage is acceptable, compared to:
where the side effect of the call seems surprising.
Nevertheless I am not going to modify the second example, maybe sometimes it is necessary.
There was a problem hiding this comment.
I find the *Aware design pattern a footgun. As is visibly the case here, it is not clear where one should check the instance type and on which values. The more such a contract is used, the more the probability that an instanceof check is missing.
I sadly need to agree that your usage is acceptable given current state of the things.
Due to the changes in #2278 `LoggerContextAware` lookups stopped working in `2.23.0`. This PR: * fixes the NPE in `Interpolator` that occurs if `Interpolator#setLoggerContext` was **not** called after instantiation. * Calls `Interpolator#setConfiguration` and `Interpolator#setLoggerContext` wherever it is possible. * Changes the way `Interpolator` propagates `Configuration` and `LoggerContext` to child lookups. Previously this occurred at each evaluation, now it occurs only in the setters. Closes #2309.
ppkarwasz
force-pushed
the
fix/2309_logger_context_aware_lookups
branch
from
9cdb359 to
395a8f4
Compare
3 participants
Due to the changes in #2278
LoggerContextAwarelookups stopped working in2.23.0.This PR:
Interpolatorthat occurs ifInterpolator#setLoggerContextwas not called after instantiation.Interpolator#setConfigurationandInterpolator#setLoggerContextwherever it is possible.InterpolatorpropagatesConfigurationandLoggerContextto child lookups. Previously this occurred at each evaluation, now it occurs only in the setters.Closes #2309.